-
Notifications
You must be signed in to change notification settings - Fork 627
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Expose features from rand to avoid compilation breakage on WASM #1804
Conversation
Current error: ``` error: target is not supported, for more information see: https://docs.rs/getrandom/#unsupported-targets --> /home/user/.cargo/registry/src/git.luolix.top-1ecc6299db9ec823/getrandom-0.1.9/src/lib.rs:249:9 | 249 | / compile_error!("\ 250 | | target is not supported, for more information see: \ 251 | | https://docs.rs/getrandom/#unsupported-targets\ 252 | | "); | |___________^ ``` In order to use `join!` and `select!` macros, we require the user to enable the `async-await` and `nightly` features. However those features will pull in rand. In order to work on WASM rand needs to have either the `wasm-bindgen` or `stdweb` feature enabled. Otherwise compile time breakage happens due to the `getrandom` crate. Explanation can be found here: https://docs.rs/getrandom/0.1.9/getrandom/#unsupported-targets This commit exposes those features through `futures-util`, allowing the end user to enable them when compiling on WASM. This means futures does not need to choose which feature to enable which would take away that decision from the end user. An alternative would be to detect the wasm32 target in Cargo.toml and to choose one of the features automatically. Current approach requires action on part of the user to enable one of those features, which means it should probably be documented somewhere. Currently the documentation of `join!` and `select!` does not mention the need for the `async-await` and `nightly` features. I propose we add some documentation changes to this PR before merging. On some quick testing this alleviates the compile error on my system, but it would be good if someone else had a look, or if we had CI testing for WASM.
Ps: another solution would be to see with |
I am not sure if you need |
@newpavlov The problem is that futures-util pulls in rand which pulls in getrandom. So the end user doesn't necessarily have getrandom in their Cargo.toml. They will depend on futures however. I just verified and adding getrandom with the wasm-bindgen feature in my crate fixes the issue without modification to futures. If that is the desired approach, we need to document this! |
Yes, I think it's the desired approach. If you have ideas of how we can improve |
Ok, I filed rust-random/getrandom#89 I still think we should add section about "target platforms" on the readme of futures, and one explaining the "feature flags" too... Also wondering, does this mean that if I have an application that depends on |
I think having to re-document this in every crate that might transitively depend on (I would not consider |
@Nemo157 Have you seen the discussion on the getrandom repo? I am proposing some improvements to the docs there. It turns out that this is a features application devs have to put, even if getrandom get's pulled in by a dependency. I would propose that we do put a section about WASM in the readme where we explain things like this, but if the docs of getrandom are better, maybe the inconvenience is bearable, but I know I try to make docs for my crates so that users know everything they should in order to use them. I shall close this PR to avoid confusion. |
Current error:
In order to use
join!
andselect!
macros, we require the user to enable theasync-await
andnightly
features. However those features will pull inrand. In order to work on WASM rand needs to have either the
wasm-bindgen
or
stdweb
feature enabled. Otherwise compile time breakage happens due to thegetrandom
crate.Explanation can be found here: https://docs.rs/getrandom/0.1.9/getrandom/#unsupported-targets
This commit exposes those features through
futures-util
, allowing the end user to enable themwhen compiling on WASM. This means futures does not need to choose which feature to enable
which would take away that decision from the end user.
An alternative would be to detect the wasm32 target in Cargo.toml and to choose
one of the features automatically.
Current approach requires action on part of the user to enable one of those features,
which means it should probably be documented somewhere. Currently the documentation
of
join!
andselect!
does not mention the need for theasync-await
andnightly
features.
I propose we add some documentation changes to this PR before merging. Following a similar
approach to getrandom by generating a compile time error to explain the situation to the user
might be the lowest friction for the end user. eg. when feature
async-await
is enabled on wasm32, verifythat one of the two required features is enabled as well, if not throw error. I can add that here
if it seems a desirable solution.
I have not tested the emscripten and WASI targets.
On some quick testing this alleviates the compile error on my system, but it would
be good if someone else had a look, or if we had CI testing for WASM.